Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR refactors how
parent_context
is handled between therender_html
andget_context_data
methods.Previously, the
render_html
method was accepting theparent_context
to be optional, defaulting toNone
. If theparent_context
isNone
it is overridden to be an emptyContext
object. That context object is then used to callget_context_data
.Usually, the
render_html
method is called from thecomponent
template tag. The template tag always passes aContext
object as theparent_context
. It never passesNone
.The
get_context_data
method required aparent_context
to be passed, it does not have a default value for the argument. But, the base implementation inComponent.get_context_data
just ignores theparent_context
object and returns an empty dict.This setup has the effect that when child classes override the
get_context_data
that theirsuper().get_context_data(parent_context)
call require theparent_context
argument. But they only get an empty dict. This seems confusing and unnecessary, since we ignore theparent_context
anyways. The only reason it's there is so that child components can make use of it.Also, by always passing a
Context
object as theparent_context
fromrender_html
to theget_context_data
, this suggest that this would really be a parent context, which typically includes things like the request object. However, we cannot rely on the request to be in theparent_context
becauserender_html
might have been called withNone
and just gives us an emptyContext
.This inconsistency between the
parent_context
that is passed torender_html
vs the one passed toget_context_data
could easily lead to confusion in the case where we callrender_html
directly and don't pass aparent_context
(as opposed it being called by thecomponent
tag which always passes the parent context).This PR tries to make the
parent_context
betweenrender_html
andget_context_data
more consistent. To do so, theparent_context
argument ofget_context_data
now defaults toNone
. We still just return an empty dict. We don't override theparent_context
in therender_html
method anymore.This means that all decisions about the
parent_context
can now be made in theget_context_data
method. This feels like the appropriate place for all handling about context data, as the method is accordingly named.The only case where this refactor would require a change in usage (of which there is not much at this point) is when the
render_html
method is called directly without passing a parent context andget_context_data
expects to pull some data out of theparent_context
. This case would need to guard against theparent_context
beingNone
.I have checked the Wagtail code base and found no usage conflicting with this change. The Wagtail test suite also passes just fine. Therefore, I consider this change as "Wagtail-compatible", which is the only "backward-compatibility" I am concerned with at this stage.
Checklist